Skip to content

UCP/PROTO: RMA and AMO restart#8547

Merged
yosefe merged 1 commit intoopenucx:masterfrom
evgeny-leksikov:ucp_proto_rma_amo_restart
Sep 29, 2022
Merged

UCP/PROTO: RMA and AMO restart#8547
yosefe merged 1 commit intoopenucx:masterfrom
evgeny-leksikov:ucp_proto_rma_amo_restart

Conversation

@evgeny-leksikov
Copy link
Contributor

What

RMA and AMO proto_v2 request restart support

if (!(req->flags & UCP_REQUEST_FLAG_PROTO_INITIALIZED)) {
pack_arg(req, op_size);
if (!(req->flags & UCP_REQUEST_FLAG_PROTO_AMO_PACKED)) {
pack_arg(req, op_size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also need to update ucp_atomic_op_nbx so it will return UCS_OK only if UCP_REQUEST_FLAG_PROTO_AMO_PACKED flag is set, and then also (for debug build) set buffer to NULL to make sure it's not used anymore

@evgeny-leksikov evgeny-leksikov force-pushed the ucp_proto_rma_amo_restart branch from 0f3e7c2 to 6250b6c Compare September 22, 2022 19:52
ep, &ucp_rkey_config(ep->worker, rkey)->proto_select,
rkey->cfg_index, req, op_id, buffer, 1, param->datatype,
op_size, param, 0, 0);
if (ucs_likely(req->flags & UCP_REQUEST_FLAG_PROTO_AMO_PACKED)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to set AMO_PACKED flag only for t protocols, to avoid the branch on "op_id == UCP_OP_ID_AMO_POST" in line 264?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on if pack_arg must be called for fetch (non-post) ops as well. As far as I can see, in current implementation, it does.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe introduce another flag , UCP_REQUEST_FLAG_PROTO_AMO_COMPLETED?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it's confusing since buffered (packed) value actually does not complete operation. Also such flag will have different behavior for different ops.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@yosefe
Copy link
Contributor

yosefe commented Sep 25, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@yosefe yosefe merged commit 4c3f14a into openucx:master Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants